EDU-3510: Python: Added paragraph that warns about pytest.fixture(scope="session") with test server#3113
EDU-3510: Python: Added paragraph that warns about pytest.fixture(scope="session") with test server#3113edmondop wants to merge 8 commits intotemporalio:mainfrom
Conversation
jsundai
added
the
community-contributor
|
Created SDK-2941 to review |
fairlydurable
changed the title
|
|
||
|
|
||
| # This might break because it will reuse the workflow environment | ||
| pytest.fixture(scope="session") |
There was a problem hiding this comment.
| pytest.fixture(scope="session") | |
| @pytest.fixture(scope="session") |
If you do keep this, need @
Co-authored-by: Chad Retz <chad.retz@gmail.com>
Co-authored-by: Chad Retz <chad.retz@gmail.com>
| # environment will be created for each test. | ||
| @pytest.fixture | ||
| def workflow_environment(): | ||
| return WorkflowEnvironment.start_time_skipping() |
There was a problem hiding this comment.
I am not sure this works without @pytest_asyncio.fixture and async and await. Have you tested this code? And for many, they may prefer a "session" scoped start_local instead of time skipping even though this only demonstrates time skipping.
There was a problem hiding this comment.
That's a good point, was using the auto-mode in my tests https://pytest-asyncio.readthedocs.io/en/latest/reference/configuration.html but I agree that being explicit works better
There was a problem hiding this comment.
It appears the workflow_environment() function is now duplicated in this snippet
There was a problem hiding this comment.
It is to show the good and bad example, do you think one of the two fixtures should be named differently ?
There was a problem hiding this comment.
Yeah, I think it's a bit confusing to even have a bad example unless it's labeled more clearly and in a separate snippet. Many of our users just grab the first snippet and move on.
So in this case, we do want start_local to be used by users and used across tests, but if they need start_time_skipping you are correct that it should be per test. So maybe we should have both forms, named differently, with comments above each on why start-local is session scoped and start-time-skipping is not.
Co-authored-by: Chad Retz <chad.retz@gmail.com>
|
@edmondop Thanks for all your work on this. Please ping me and let me know when it is ready to move forward. Cheers! |
fairlydurable
added
the
waiting-on-tech-review
|
@fairlydurable - sorry I missed your comment until now. I think https://github.com/temporalio/documentation/pull/3113/files#r1866332621 still needs to be addressed |
|
Closing this one. |
6 participants
What does this PR do?
Addresses #3111 . When running tests in a project I noticed:
This additional paragraph should warn developers familiar with pytest.fixtures not to use the scope session for caching the workflow environment